Conversation
…and remember background color
…rors properly Fixes #77
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds guarded screensaver HMI header and empty CPP stub; updates screensaver page scripts (persistent background color, deferred brightness, restart mode), tightens datetime validation/formatting and strftime safety, tweaks display/navigation and watchdog logging, hardens blueprint entity resolution and bumps blueprint/version requirement. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as PageChange Event
participant Script as page_screensaver (ESPhome script)
participant Nextion as Nextion HMI
participant HW as Hardware Refresh
participant Time as Time Provider
participant Bright as Brightness Script
Event->>Script: enter page == "screensaver"
Script->>Nextion: set background/foreground/font (if changed)
Script->>HW: trigger hardware refresh
Script->>Time: request now (if display_time enabled)
Time-->>Script: now (valid/invalid)
alt now valid
Script->>Nextion: update time text
else now invalid
Script-->>Event: log warning, skip time update
end
Script->>Bright: start page_screensaver_set_brightness_with_wait (restart)
Bright->>Bright: wait 5s
Bright->>Nextion: set brightness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/nspanel_easy/page_screensaver.h (1)
35-35: Derive the page ID instead of duplicating9.
pages.honly guarantees that"screensaver"exists. Hard-coding the index here can silently drift if the page table is reordered later.Suggested change
-constexpr HMIComponent PAGE = {"screensaver", 9}; ///< Screensaver page (index 9 in page_names array) +constexpr HMIComponent PAGE = {"screensaver", get_page_id("screensaver")}; ///< Screensaver page🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/nspanel_easy/page_screensaver.h` at line 35, PAGE is hard-coded with ID 9; instead look up the "screensaver" page ID from the canonical pages table in pages.h at compile or init time rather than duplicating the literal. Replace the constexpr initialization of PAGE in page_screensaver.h so it derives its id by calling the existing lookup/mapper (e.g., a constexpr get_page_id or find_page_index function that takes "screensaver" or by referencing the page_names/page_table entry) and use that result to construct PAGE; ensure you reference the exact symbol "screensaver" and the lookup utility from pages.h so the value follows any reorderings of the page table.esphome/nspanel_esphome_page_screensaver.yaml (1)
176-190: Letpage_screensaverown the sleep brightness update.
page_change -> page_screensaveralready applies the sleep brightness immediately and schedules the 5s retry.Line 190duplicates that write and can dim the current page before the Nextion page change is actually confirmed.Suggested change
ESP_LOGD("${TAG_PAGE_SCREENSAVER}", "Sleep from '%s'", page_names[current_page_id]); goto_page->execute(get_page_id("screensaver")); - set_brightness->execute(display_sleep_brightness->state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_page_screensaver.yaml` around lines 176 - 190, Duplicate brightness write: the timer_sleep routine currently calls set_brightness->execute(display_sleep_brightness->state) which duplicates the page_change -> page_screensaver behavior and can dim the current page prematurely; remove the set_brightness->execute(...) call from the timer_sleep lambda/then block (keep the timeout checks, delay and goto_page->execute(get_page_id("screensaver")) logic intact) so that page_screensaver solely owns updating brightness and the 5s retry logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@esphome/nspanel_esphome_datetime.yaml`:
- Around line 154-163: refresh_datetime() currently formats
id(time_provider).now() without checking validity, so invalid clock values can
still be pushed; update refresh_datetime() to call auto now =
id(time_provider).now() and check now.is_valid() (same as render_date()) before
formatting or updating UI, and if invalid log a warning (similar message used in
render_date) and return/skip the update so no invalid time text is rendered.
---
Nitpick comments:
In `@components/nspanel_easy/page_screensaver.h`:
- Line 35: PAGE is hard-coded with ID 9; instead look up the "screensaver" page
ID from the canonical pages table in pages.h at compile or init time rather than
duplicating the literal. Replace the constexpr initialization of PAGE in
page_screensaver.h so it derives its id by calling the existing lookup/mapper
(e.g., a constexpr get_page_id or find_page_index function that takes
"screensaver" or by referencing the page_names/page_table entry) and use that
result to construct PAGE; ensure you reference the exact symbol "screensaver"
and the lookup utility from pages.h so the value follows any reorderings of the
page table.
In `@esphome/nspanel_esphome_page_screensaver.yaml`:
- Around line 176-190: Duplicate brightness write: the timer_sleep routine
currently calls set_brightness->execute(display_sleep_brightness->state) which
duplicates the page_change -> page_screensaver behavior and can dim the current
page prematurely; remove the set_brightness->execute(...) call from the
timer_sleep lambda/then block (keep the timeout checks, delay and
goto_page->execute(get_page_id("screensaver")) logic intact) so that
page_screensaver solely owns updating brightness and the 5s retry logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc8ab32e-2a48-4eec-bade-0e11368a396b
📒 Files selected for processing (8)
components/nspanel_easy/page_screensaver.cppcomponents/nspanel_easy/page_screensaver.hesphome/nspanel_esphome_base.yamlesphome/nspanel_esphome_datetime.yamlesphome/nspanel_esphome_hw_display.yamlesphome/nspanel_esphome_localization.yamlesphome/nspanel_esphome_page_screensaver.yamlnspanel_easy_blueprint.yaml
This was
linked to
issues
Mar 30, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
esphome/nspanel_esphome_datetime.yaml (1)
154-167:⚠️ Potential issue | 🟠 Majorpage_weather.yaml passes unsynchronized timestamps without validation.
The
page_weatherautomation at line 79 synthesizes atimestampfromid(time_provider).now().timestampwithout checkingnow.is_valid()first. When passed torender_date->execute(), this timestamp may reconstruct into a spurious 1970-era date if the clock is not yet synchronized, since therender_datefunction only validates after callingESPTime::from_epoch_local().The datetime page's call at line 108 is safe—it guards
now.is_valid()much earlier in the same lambda before callingstrftime()and before passing the timestamp.Add an
if (!now.is_valid()) return;guard in thepage_weatherlambda before calculating and passing the synthesizedtimestamp:const ESPTime now = id(time_provider).now(); if (!now.is_valid()) return; const int32_t seconds_to_noon = (12 - static_cast<int32_t>(now.hour)) * 3600; const uint32_t timestamp = static_cast<uint32_t>( static_cast<int64_t>(now.timestamp) + seconds_to_noon + (page_index * ${SECONDS_PER_DAY}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esphome/nspanel_esphome_datetime.yaml` around lines 154 - 167, In the page_weather lambda that calls id(time_provider).now() and synthesizes a timestamp to pass to render_date->execute(), add a guard checking now.is_valid() and return early if false; specifically, after obtaining const ESPTime now = id(time_provider).now(); insert if (!now.is_valid()) return; before computing seconds_to_noon, timestamp, or calling render_date->execute(), so you never build or pass an epoch-derived timestamp from an unsynchronized clock.
🧹 Nitpick comments (2)
components/nspanel_easy/page_screensaver.h (2)
25-32: Minor: Hardcoded page index in documentation.Line 29 references "index 9 in page_names array" which could become stale if the array order changes. Consider removing the specific index or noting it's determined at compile-time via
get_page_id().That said, the
static_assertinpages.hensures compile-time validation, so this is purely a documentation hygiene concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/nspanel_easy/page_screensaver.h` around lines 25 - 32, Update the header comment in page_screensaver.h to remove the hardcoded "index 9 in page_names array" and instead state that the page index is determined at compile-time (via get_page_id()) and validated by the static_assert in pages.h; locate the comment block that mentions "index 9" and replace that phrase with a neutral description referencing get_page_id() and the static_assert in pages.h for compile-time validation.
47-50: Clarification:PAGEinALL[]array.The comment mentions "All visual components" but
PAGEis the page identifier itself rather than a visual component. If this is intentional (e.g., for setting page-level background properties during iteration), the current approach is fine. Otherwise, consider whetherALL[]should only contain{TEXT}.This is a minor observation and likely by design for your iteration use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/nspanel_easy/page_screensaver.h` around lines 47 - 50, The ALL[] array currently contains PAGE and TEXT but PAGE is a page identifier rather than a visual component; either remove PAGE from ALL so it becomes just {TEXT} and update COMPONENT_COUNT accordingly (change ALL and recompute sizeof), or if PAGE must be iterated (e.g., to apply page-level background properties) add a clarifying comment above ALL explaining why PAGE is included; update the declaration of ALL and the COMPONENT_COUNT definition (symbols: ALL, PAGE, TEXT, COMPONENT_COUNT) to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@esphome/nspanel_esphome_datetime.yaml`:
- Around line 154-167: In the page_weather lambda that calls
id(time_provider).now() and synthesizes a timestamp to pass to
render_date->execute(), add a guard checking now.is_valid() and return early if
false; specifically, after obtaining const ESPTime now =
id(time_provider).now(); insert if (!now.is_valid()) return; before computing
seconds_to_noon, timestamp, or calling render_date->execute(), so you never
build or pass an epoch-derived timestamp from an unsynchronized clock.
---
Nitpick comments:
In `@components/nspanel_easy/page_screensaver.h`:
- Around line 25-32: Update the header comment in page_screensaver.h to remove
the hardcoded "index 9 in page_names array" and instead state that the page
index is determined at compile-time (via get_page_id()) and validated by the
static_assert in pages.h; locate the comment block that mentions "index 9" and
replace that phrase with a neutral description referencing get_page_id() and the
static_assert in pages.h for compile-time validation.
- Around line 47-50: The ALL[] array currently contains PAGE and TEXT but PAGE
is a page identifier rather than a visual component; either remove PAGE from ALL
so it becomes just {TEXT} and update COMPONENT_COUNT accordingly (change ALL and
recompute sizeof), or if PAGE must be iterated (e.g., to apply page-level
background properties) add a clarifying comment above ALL explaining why PAGE is
included; update the declaration of ALL and the COMPONENT_COUNT definition
(symbols: ALL, PAGE, TEXT, COMPONENT_COUNT) to reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23ad91d2-84a4-4c09-85bb-b189271108ec
📒 Files selected for processing (4)
components/nspanel_easy/page_screensaver.hesphome/nspanel_esphome_datetime.yamlesphome/nspanel_esphome_version.yamlnspanel_easy_blueprint.yaml
✅ Files skipped from review due to trivial changes (1)
- esphome/nspanel_esphome_version.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- nspanel_easy_blueprint.yaml
This was referenced Mar 31, 2026
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements